Fix standard library symbols completion (solves #9760) #9805
Conversation
|
I haven't checked but I assume this regressed in 6990aab, /cc @TheElectronWill |
| @@ -1284,7 +1284,9 @@ class Definitions { | |||
|
|
|||
| private val ScalaImportFns: List[RootRef] = | |||
| JavaImportFns :+ | |||
| RootRef(() => ScalaPackageVal.termRef) | |||
| RootRef(() => ScalaPackageVal.termRef) :+ | |||
| RootRef(() => ScalaPackageObject.termRef) | |||
There was a problem hiding this comment.
This just makes compilation slower. An import from ScalaPackageVal will automatically import members of scala.package. On the other hand, now we have one more import to search for every root reference.
What breaks if we do not do this change?
There was a problem hiding this comment.
Without this change the PR doesn't fix anything. But I agree that we shouldn't need it.
There was a problem hiding this comment.
As stated in the PR description, this PR fixes 2 issues (caused by 2 separate things in the code base but influencing users in a similar way):
- Completion of symbols from scala package object, like
SeqorBigDecimal - Completion of other predefined symbols (from Predef and top level definitions from scala package), like
println
The latter issue is indeed a regression introduced in the PR that @smarter mentioned and it occurs only in LSP. The former issue must be older, even though nobody else seems to have spotted it before, it affects both LSP and REPL and is fixed specifically by this line (although this itself wouldn't fix LSP without the other fix).
I'm not really sure how the problem could be solved without adding this line. Interestingly this only changes the behaviour of symbols completion. Symbols like Seq or BigDecimal cause no problems in compilation and symbols completion should probably have exactly the same view of symbols in scope as the compiler.
There was a problem hiding this comment.
Ah right, I forgot the other issue. Adding root imports to the context like you did is indeed the right solution for this second issue.
There was a problem hiding this comment.
Actually, just adding RootRef(() => ScalaPackageObject.termRef) to ScalaImportFns solves the issue (for the REPL, at least). And adding scala root imports to the rootContext (to end up in a state similar to before #9394) doesn't. So the real issue seems to be somewhere else 🤔
There was a problem hiding this comment.
I think the addition of ScalaPackageObject in the root import is a workaround for an underlying but that needs to be fixed: when importing a package, we should also get completions for members of the package object. For example:
import scala.collection._
val foo: TraversableOnc<TAB>This should find TraversableOnce since it's defined in https://github.com/scala/scala/blob/d003bf5159acaca883f90e1be822bef9316639de/src/library/scala/collection/package.scala#L23, but it doesn't find anything. If I explicitly import the package object it works:
import scala.collection.`package`.
val foo: TraversableOnc<TAB>So the completion logic needs to be fixed to handle package objects correctly.
prolativ
force-pushed
the
fix-standard-library-symbols-completion
branch
2 times, most recently
from
2d7881d to
f1cb964
Compare
| @@ -264,7 +264,12 @@ object Completion { | |||
| private def accessibleMembers(site: Type)(using Context): Seq[Symbol] = site match { | |||
| case site: NamedType if site.symbol.is(Package) => | |||
| // Don't look inside package members -- it's too expensive. | |||
There was a problem hiding this comment.
I would like to get this PR finally merged but I'm not sure what I should do to push it forward. If the comment is still valid, I can revert some of the changes and leave only the fix for the regression but from user's perspective completion not working for symbols like List or Iterator might be quite confusing and irritating.
There was a problem hiding this comment.
We might do it on the Metals side, but it will require a bit more work still.
There was a problem hiding this comment.
Just asked Martin IRL about this and he thinks it's fine to make this change. So LGTM from me, but please remove that comment since it's no longer accurate.
prolativ
force-pushed
the
fix-standard-library-symbols-completion
branch
from
f1cb964 to
138c742
Compare
Symbols which are available in scope by default like
printlnorRunnablewere not suggested by LSP completion feature.Some symbols like
SeqorBigDecimalwere suggested neither by LSP nor in REPL.This PR solves both these issues.